src/main/java/net/frc5183/librobot/math/curve/Curve.java (2)

3-5: Consider enhancing the class-level Javadoc.

While the current Javadoc is clear and concise, it could be more informative. Consider adding details about:

1. The expected usage of this abstract class.
2. Any implementation requirements for subclasses.
3. The significance of this class in the context of the robotics library.

This additional information would provide better guidance for developers working with or extending this class.

Here's a suggested enhancement:

/**
 * An abstract class which represents a mathematical function/curve.
 * <p>
 * This class serves as a base for various curve implementations in the robotics library.
 * Subclasses should implement the {@link #curve(Double)} method to define specific
 * mathematical behaviors.
 * <p>
 * Typical usage involves creating concrete subclasses for different types of curves
 * (e.g., linear, quadratic, exponential) and using them for calculations in robot control systems.
 */

---

7-11: Consider enhancing the method-level Javadoc.

The current Javadoc for the curve method is good, but it could be improved with some additional details:

1. Mention any potential exceptions that might be thrown (if applicable).
2. Provide information about how the method handles edge cases or special values (e.g., null, infinity, NaN).
3. If there are any constraints on the input or output, they should be documented.

Here's a suggested enhancement:

/**
 * Returns the value of the curve at the given x value.
 *
 * @param x The x value to evaluate the curve at.
 * @return The value of the curve at the given x value.
 * @throws NullPointerException if x is null
 * @implSpec Implementations should handle special cases such as infinity and NaN appropriately.
 * The behavior for these cases should be clearly defined in the implementing class.
 */

src/main/java/net/frc5183/librobot/math/curve/ExponentialCurve.java (3)

3-7: Enhance the class-level Javadoc for clarity.

The Javadoc comment provides a good overview of the class purpose and the mathematical formula. However, it could be improved for better clarity and understanding.

Consider updating the Javadoc as follows:

/**
 * An {@link Curve} implementation representing an exponential function.
 * <p>
 * The curve is defined by the equation:
 * y = (x/|x|) * ((1 + k)^|x| - 1) / k
 * <p>
 * Where:
 * <ul>
 *   <li>x is the input value</li>
 *   <li>k is the exaggeration parameter of the curve</li>
 * </ul>
 * This curve provides a customizable exponential response with the exaggeration parameter.
 */

This enhancement provides a clearer structure and explains the variables in the equation.

---

13-19: Minor enhancement to constructor Javadoc.

The constructor's Javadoc is good, but we can improve it slightly for better documentation.

Consider updating the Javadoc as follows:

/**
 * Creates a new {@link ExponentialCurve} with the specified exaggeration.
 * 
 * @param exaggeration The exaggeration parameter of the curve. Higher values result in a more pronounced curve.
 */

This change provides more context about the exaggeration parameter's effect on the curve.

---

21-27: Approve implementation with minor suggestion.

The curve method correctly implements the exponential curve formula and handles the special case for x = 0. The use of Math.abs() and Math.pow() is appropriate.

Consider adding braces to the if statement for consistency and to follow best practices:

@Override
public Double curve(Double x) {
    if (x == 0) {
        return 0d;
    }

    return (x / Math.abs(x)) * ((Math.pow(1 + exaggeration, Math.abs(x))) - 1) / exaggeration;
}

This change addresses the PMD warning about control statement braces and improves code consistency.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 23-23: This statement should have braces
Enforce a policy for braces on control statements. It is recommended to use braces on 'if ... else'
statements and loop statements, even if they are optional. This usually makes the code clearer, and
helps prepare the future when you need to add another statement. That said, this rule lets you control
which statements are required to have braces via properties.

From 6.2.0 on, this rule supersedes WhileLoopMustUseBraces, ForLoopMustUseBraces, IfStmtMustUseBraces,
and IfElseStmtMustUseBraces.

ControlStatementBraces (Priority: 3, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#controlstatementbraces

---

[warning] 26-26: Useless parentheses.
Parenthesized expressions are used to override the default operator precedence
rules. Parentheses whose removal would not change the relative nesting of operators
are unnecessary, because they don't change the semantics of the enclosing expression.

Some parentheses that strictly speaking are unnecessary, may still be considered useful
for readability. This rule allows to ignore violations on two kinds of unnecessary parentheses:

• "Clarifying" parentheses, which separate operators of difference precedence. While
  unnecessary, they make precedence rules explicit, which may be useful for rarely used
  operators. For example:

    (a + b) & c // is equivalent to `a + b & c`, but probably clearer

Unset the property ignoreClarifying to report them.

• "Balancing" parentheses, which are unnecessary but visually balance out another pair
  of parentheses around an equality operator. For example, those two expressions are equivalent:

    (a == null) != (b == null)
    a == null != (b == null)

The parentheses on the right are required, and the parentheses on the left are
just more visually pleasing. Unset the property ignoreBalancing to report them.

UselessParentheses (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#uselessparentheses

---

[warning] 26-26: Useless parentheses.
Parenthesized expressions are used to override the default operator precedence
rules. Parentheses whose removal would not change the relative nesting of operators
are unnecessary, because they don't change the semantics of the enclosing expression.

Some parentheses that strictly speaking are unnecessary, may still be considered useful
for readability. This rule allows to ignore violations on two kinds of unnecessary parentheses:

• "Clarifying" parentheses, which separate operators of difference precedence. While
  unnecessary, they make precedence rules explicit, which may be useful for rarely used
  operators. For example:

    (a + b) & c // is equivalent to `a + b & c`, but probably clearer

Unset the property ignoreClarifying to report them.

• "Balancing" parentheses, which are unnecessary but visually balance out another pair
  of parentheses around an equality operator. For example, those two expressions are equivalent:

    (a == null) != (b == null)
    a == null != (b == null)

The parentheses on the right are required, and the parentheses on the left are
just more visually pleasing. Unset the property ignoreBalancing to report them.

UselessParentheses (Priority: 4, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#uselessparentheses

src/main/java/net/frc5183/librobot/math/curve/NormalTimedCurve.java (2)

1-6: Enhance class documentation for clarity.

The class documentation provides a concise description of the functionality. However, it could be improved by adding more details about the purpose and usage of the class.

Consider expanding the class documentation to include:

1. A brief explanation of what a TimedCurve is.
2. The specific use case or scenario where NormalTimedCurve would be beneficial.
3. An example of how the class toggles between x and curve(x).

Here's a suggested improvement:

/**
 * A {@link TimedCurve} that toggles between x (when disabled) and curve(x) (when enabled).
 * This class is useful in scenarios where you need to apply a curve transformation
 * to an input value for a specific duration, then revert to the original input.
 * 
 * For example, it can be used in robotics control systems to temporarily modify
 * input values based on certain conditions or time intervals.
 * 
 * When enabled: output = curve(x)
 * When disabled: output = x
 */

---

23-31: LGTM with a suggestion: Consider adding null check in enabled method.

The implementations of enabled and disabled methods are correct and align with the class description. They are concise and clear in their purpose.

However, to improve robustness, consider adding a null check in the enabled method:

 @Override
 protected Double enabled(Double x) {
+    if (x == null) {
+        return null; // or throw an exception, depending on your error handling strategy
+    }
     return curve(x);
 }

This change will prevent potential NullPointerExceptions if the input x is null.

src/main/java/net/frc5183/librobot/math/curve/LinearCurve.java (1)

33-63: LGTM: Getter and setter methods are well-implemented.

The getter and setter methods for slope and yIntercept are correctly implemented with clear Javadoc comments.

Consider adding validation in the setter methods to ensure that only valid values are set. For example:

public void setSlope(double slope) {
    if (Double.isFinite(slope)) {
        this.slope = slope;
    } else {
        throw new IllegalArgumentException("Slope must be a finite number");
    }
}

This would prevent setting invalid values like NaN or Infinity.

src/main/java/net/frc5183/librobot/math/curve/RadicalCurve.java (3)

3-6: Enhance class-level Javadoc comment

The current Javadoc comment provides a good basic description of the class. However, it could be improved by adding more details about the parameters and their constraints.

Consider expanding the Javadoc comment to include:

• Descriptions of A, B, and C parameters
• Any constraints on these parameters (e.g., A != 0)
• The domain and range of the function

Example:

/**
 * A {@link Curve} which represents a radical equation in the form y = (x+C)^(1/A)+B.
 * 
 * @param A The root degree (A != 0)
 * @param B The vertical shift
 * @param C The horizontal shift
 * 
 * Domain: x >= -C when A > 0, x <= -C when A < 0
 * Range: y >= B when A > 0, y <= B when A < 0
 */

---

22-32: Enhance constructor Javadoc and consider adding parameter validation

The constructor's Javadoc comment could be more informative about parameter constraints.

1. Enhance the Javadoc comment to include parameter constraints. For example:

/**
 * Creates a new {@link RadicalCurve} with the given A, B, and C values.
 * @param a The "A" variable in the curve equation. Must not be zero.
 * @param b The "B" variable in the curve equation.
 * @param c The "C" variable in the curve equation.
 * @throws IllegalArgumentException if a is zero.
 */

2. Consider adding parameter validation in the constructor:

public RadicalCurve(Double a, Double b, Double c) {
    if (a == 0) {
        throw new IllegalArgumentException("Parameter 'a' must not be zero.");
    }
    this.a = a;
    this.b = b;
    this.c = c;
}

---

40-86: Enhance Javadoc comments for setter methods

The current Javadoc comments for the setter methods could be more informative about the mathematical implications of changing these values.

Consider updating the Javadoc comments for the setter methods to include more details. For example:

/**
 * Sets the "A" variable in the curve equation.
 * Changing this value affects the root degree of the radical function.
 * @param a The new "A" variable in the curve equation. Must not be zero.
 * @throws IllegalArgumentException if a is zero.
 */
public void setA(Double a) {
    if (a == 0) {
        throw new IllegalArgumentException("Parameter 'a' must not be zero.");
    }
    this.a = a;
    this.inverseA = 1 / a; // Update cached value
}

/**
 * Sets the "B" variable in the curve equation.
 * Changing this value shifts the entire curve vertically.
 * @param b The new "B" variable in the curve equation.
 */
public void setB(Double b) {
    this.b = b;
}

/**
 * Sets the "C" variable in the curve equation.
 * Changing this value shifts the curve horizontally and affects the domain of the function.
 * @param c The new "C" variable in the curve equation.
 */
public void setC(Double c) {
    this.c = c;
}

These enhanced comments provide more context about how changing each variable affects the curve's behavior.

src/main/java/net/frc5183/librobot/math/curve/QuadraticCurve.java (2)

1-20: Class structure looks good, consider using primitive types.

The class structure is well-designed for representing a quadratic curve. The fields are appropriately declared as private with clear comments.

Consider using primitive double instead of Double for the coefficients if null values are not needed. This would prevent potential NullPointerExceptions and improve performance. If you decide to keep Double, ensure proper null checks are in place where these fields are used.

---

48-54: Setter methods are correct, consider adding input validation.

The setter methods for a, b, and c are implemented correctly and have clear, descriptive comments.

Consider adding input validation to prevent setting null values or other invalid states. For example:

public void setA(Double a) {
    if (a == null) {
        throw new IllegalArgumentException("Coefficient 'a' cannot be null");
    }
    this.a = a;
}

Apply similar checks to setB and setC methods. This will help maintain the integrity of the QuadraticCurve object.

Also applies to: 64-70, 80-86

src/main/java/net/frc5183/librobot/math/curve/DoubleTimedCurve.java (2)

3-6: Enhance the class-level Javadoc comment

The current Javadoc provides a basic description of the class. Consider expanding it to include more details about the purpose and behavior of the DoubleTimedCurve class.

Here's a suggested improvement:

/**
 * A {@link TimedCurve} that toggles between two curves based on its enabled/disabled state.
 * This class allows for dynamic switching between two different curve behaviors,
 * defined by the enabledCurve and disabledCurve.
 */
public class DoubleTimedCurve extends TimedCurve {

---

41-63: Enhance Javadoc for getter methods

While the getter methods are correctly implemented, their Javadoc comments could be more informative. Consider adding more context about when these methods might be used.

Here's a suggested improvement for getEnabledCurve:

/**
 * Returns the curve used when this DoubleTimedCurve is in an enabled state.
 * This curve determines the behavior of the `enabled` method.
 *
 * @return The Curve object used when this DoubleTimedCurve is enabled.
 */
public Curve getEnabledCurve() {
    return enabledCurve;
}

Apply a similar improvement to getDisabledCurve.

src/main/java/net/frc5183/librobot/math/curve/TimedCurve.java (2)

34-37: Consider starting the Timer in the constructor

The constructor initializes the delay fields correctly. However, it doesn't start the Timer. Consider adding timer.start(); at the end of the constructor to ensure the Timer begins running immediately upon object creation.

 public TimedCurve(Double delayEnabled, Double delayDisabled) {
     this.delayEnabled = delayEnabled;
     this.delayDisabled = delayDisabled;
+    timer.start();
 }

---

39-50: Add a comment explaining the logic and consider using if-else for clarity

The curve method implements the core logic for toggling between enabled and disabled states correctly. However, consider adding a comment explaining the logic and using an if-else statement instead of a ternary operator for better readability.

 @Override
 public Double curve(Double x) {
+    // Check if it's time to toggle the state based on the current state and elapsed time
     if (disabled && timer.hasElapsed(delayDisabled)) {
         disabled = false;
         timer.reset();
     } else if (!disabled && timer.hasElapsed(delayEnabled)) {
         disabled = true;
         timer.reset();
     }

-    return disabled ? disabled(x) : enabled(x);
+    // Return the appropriate value based on the current state
+    if (disabled) {
+        return disabled(x);
+    } else {
+        return enabled(x);
+    }
 }

src/main/java/net/frc5183/librobot/math/curve/PiecewiseCurve.java (5)

19-50: LGTM: Constructors are well-implemented, with a minor suggestion.

The constructors provide good flexibility for creating PiecewiseCurve instances. The use of Map.of() and Map.ofEntries() is appropriate for creating immutable maps.

Consider adding a @SafeVarargs annotation to the constructor that takes a Map<Function<Double, Boolean>, Curve> parameter. While not strictly necessary, it would be consistent with the other varargs constructor and suppress any potential warnings.

+    @SafeVarargs
     public PiecewiseCurve(Map<Function<Double, Boolean>, Curve> curves) {
         this.curves = curves;
     }

---

52-60: LGTM: curve method implementation is correct, with a potential optimization.

The curve method correctly implements the piecewise function logic, iterating through the conditions and returning the value of the first matching curve. The default return value of 0d is consistent with the class documentation.

Consider using the Stream API for a more concise implementation and potential performance improvement:

     @Override
     public Double curve(Double x) {
-        for (Map.Entry<Function<Double, Boolean>, Curve> entry : curves.entrySet()) {
-            if (entry.getKey().apply(x)) {
-                return entry.getValue().curve(x);
-            }
-        }
-        return 0d;
+        return curves.entrySet().stream()
+            .filter(entry -> entry.getKey().apply(x))
+            .findFirst()
+            .map(entry -> entry.getValue().curve(x))
+            .orElse(0d);
     }

This implementation may be more efficient for large maps, as it can short-circuit the evaluation once a matching condition is found.

---

86-93: LGTM: replace method is correctly implemented, with a minor suggestion.

The replace method provides a clean way to update a curve for a given condition without exposing the internal map structure.

For consistency with other methods in the class, consider returning the previous curve (if any) associated with the condition:

-    public void replace(Function<Double, Boolean> condition, Curve curve) {
-        curves.replace(condition, curve);
+    public Curve replace(Function<Double, Boolean> condition, Curve curve) {
+        return curves.replace(condition, curve);
     }

This change would make the method more informative and align it with the behavior of java.util.Map.replace().

---

95-102: LGTM: put method is correctly implemented, with a minor suggestion.

The put method provides a clean way to add a new condition-curve pair to the piecewise curve without exposing the internal map structure.

For consistency with the java.util.Map.put() method and to provide more information to the caller, consider returning the previous curve (if any) associated with the condition:

-    public void put(Function<Double, Boolean> condition, Curve curve) {
-        curves.put(condition, curve);
+    public Curve put(Function<Double, Boolean> condition, Curve curve) {
+        return curves.put(condition, curve);
     }

This change would make the method more informative and align it with the behavior of java.util.Map.put().

---

1-127: Overall, the PiecewiseCurve class is well-designed and implemented.

The class provides a robust and efficient implementation of a piecewise curve using a map of conditions to curves. The methods offer a good balance of functionality and encapsulation, allowing for easy creation, access, and manipulation of the curve without exposing the internal map structure.

Consider adding a method to combine two PiecewiseCurve instances, which could be useful in more complex scenarios:

public PiecewiseCurve combine(PiecewiseCurve other) {
    Map<Function<Double, Boolean>, Curve> combinedCurves = new HashMap<>(this.curves);
    other.forEachEntry((condition, curve) -> combinedCurves.putIfAbsent(condition, curve));
    return new PiecewiseCurve(combinedCurves);
}

This method would allow users to create more complex piecewise curves by combining existing ones, enhancing the flexibility and composability of the class.

Configuration used: CodeRabbit UI
Review profile: CHILL

src/main/java/net/frc5183/librobot/math/curve/ExponentialCurve.java (1)

21-27: Consider adding Javadoc to the curve method

The curve method is well-implemented and correctly handles the case where x is 0. However, it would benefit from additional documentation to explain its behavior and parameters.

Consider adding a Javadoc comment to the curve method:

/**
 * Calculates the y-value of the exponential curve for a given x-value.
 * @param x The input value.
 * @return The calculated y-value of the curve.
 * @implNote Returns 0 when x is 0 to avoid division by zero.
 */
@Override
public Double curve(Double x) {
    // ... existing implementation ...
}

This additional documentation will improve the readability and maintainability of the code.

🧰 Tools

🪛 GitHub Check: pmd

[warning] 23-23: This statement should have braces
Enforce a policy for braces on control statements. It is recommended to use braces on 'if ... else'
statements and loop statements, even if they are optional. This usually makes the code clearer, and
helps prepare the future when you need to add another statement. That said, this rule lets you control
which statements are required to have braces via properties.

From 6.2.0 on, this rule supersedes WhileLoopMustUseBraces, ForLoopMustUseBraces, IfStmtMustUseBraces,
and IfElseStmtMustUseBraces.

ControlStatementBraces (Priority: 3, Ruleset: Code Style)
https://docs.pmd-code.org/pmd-doc-7.6.0/pmd_rules_java_codestyle.html#controlstatementbraces

Configuration used: CodeRabbit UI
Review profile: CHILL